Repository navigation
refactor: use MigratorTraitSelf to eliminate global migration state - #2753
Conversation
Reviewer's GuideRefactors migration execution from static SeaORM APIs plus global/task-local state to explicit Sequence diagram for explicit migration executionsequenceDiagram
participant Caller
participant Database
participant Migrator
participant SeaORM
participant DataMigration
Caller->>Migrator: new(storage, options)
Caller->>Database: migrate(migrator)
Database->>Migrator: up(database, None)
Migrator->>Migrator: migrations()
Migrator->>SeaORM: execute migrations
SeaORM->>DataMigration: up(manager)
DataMigration->>DataMigration: use storage and options
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="migration/src/main.rs" line_range="7" />
<code_context>
#[tokio::main]
async fn main() {
- cli::run_cli(migration::Migrator).await;
+ let storage = StorageConfig::parse()
+ .into_storage(false)
+ .await
</code_context>
<issue_to_address>
**Migration commands are rejected**
When the migration binary is invoked with a migration subcommand or its flags, `StorageConfig::parse()` parses the process arguments before `cli::run_cli` receives them, so migration commands such as `up` or `status` are rejected as unknown storage arguments and the migration CLI exits.
Parse storage settings without consuming the migration CLI arguments, or combine storage and migration arguments in one parser.
</issue_to_address>
### Comment 2
<location path="trustd/src/db.rs" line_range="15" />
<code_context>
pub(crate) command: Command,
#[command(flatten)]
pub(crate) database: Database,
+ /// Location of the storage
</code_context>
<issue_to_address>
**Data command flags are rejected**
When a caller supplies storage or migration-option flags after `trustify db data`, clap defines `StorageConfig` and `Options` on `Run` rather than `Data`, so previously accepted flags after `data` are rejected as unknown arguments and the requested data migration does not run.
Preserve the flattened storage and migration options on `Data`, or make them global so existing command forms remain accepted.
Also at `trustd/src/db.rs:16-18`.
</issue_to_address>
### Comment 3
<location path="migration/src/main.rs" line_range="11" />
<code_context>
+ .into_storage(false)
+ .await
+ .expect("failed to initialize storage");
+ cli::run_cli(migration::Migrator::new(storage, ())).await;
}
</code_context>
<issue_to_address>
**Configured migration options are ignored**
When a `MIGRATION_DATA_*` variable is set for the standalone migration binary, `Migrator::new(storage, ())` uses default `Options`, so configured skip and runner settings are ignored and data migrations run with defaults, including migrations the operator intended to skip.
Construct `Options` from the environment and pass it to the migrator.
Also at `server/src/profile/api.rs:465-466`, `xtask/src/dataset.rs:131`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and the refactor changes which storage backend and options data migrations use, so an incorrect wiring could write incorrect derived values or leave migrations incomplete in the database. Reverting restores the previous runner, but any already-written values would need to be recomputed or the affected migration rerun.
Blocking findings: migration/src/main.rs:7, trustd/src/db.rs:15, migration/src/main.rs:11
| #[command(flatten)] | ||
| pub(crate) database: Database, | ||
| /// Location of the storage | ||
| #[command(flatten)] |
There was a problem hiding this comment.
🟡 Medium · Data command flags are rejected
When a caller supplies storage or migration-option flags after trustify db data, clap defines StorageConfig and Options on Run rather than Data, so previously accepted flags after data are rejected as unknown arguments and the requested data migration does not run.
Preserve the flattened storage and migration options on Data, or make them global so existing command forms remain accepted.
Also at trustd/src/db.rs:16-18.
Prompt for AI agents
In `trustd/src/db.rs` at line 15:
**Data command flags are rejected**
When a caller supplies storage or migration-option flags after `trustify db data`, clap defines `StorageConfig` and `Options` on `Run` rather than `Data`, so previously accepted flags after `data` are rejected as unknown arguments and the requested data migration does not run.
Preserve the flattened storage and migration options on `Data`, or make them global so existing command forms remain accepted.
Also at `trustd/src/db.rs:16-18`.…ion state Replace the LazyLock/task_local side channel used to smuggle storage and options into data migrations with SeaORM 2.x's MigratorTraitSelf trait. Migrator is now a struct carrying DispatchBackend and Options as fields. The instance method migrations(&self) constructs MigrationWithData wrappers directly from the migrator's state, removing the need for global statics, task-local scoping, and the run_with_test() helper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1b54261 to
5d64ab4
Compare
StorageConfig::parse() consumed all CLI args before SeaORM's migration CLI could see them, rejecting subcommands like `up` and `status`. Additionally, Migrator::new(storage, ()) ignored MIGRATION_DATA_* env vars by using default Options. Replace the dual-parse approach with a single combined clap parser that flattens StorageConfig, Options, and SeaORM's MigrateSubcommands, then calls cli::run_migrate directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rh-jfuller
left a comment
There was a problem hiding this comment.
a few things to comment on:
Dev mode now ignores the MIGRATION_DATA_* environment variables eg. MIGRATION_DATA_SKIP_ALL=true trustd api --devmode will now run every data migration. I think the same applies to MIGRATION_DATA_SKIP and MIGRATION_DATA_CONCURRENT ... is this what we want ?
The migration binary lost three things that sea-orm’s run_cli use to provide .env loading, init and generate without db and we get errors now instead of panics (which is good) ... if this is what we want then cool, though pointing out the change.
last thing I wanted to point out was that command line flags must now come before the subcommand ... easy enough to fix order of flags.
Summary
Migratoris now a struct carryingDispatchBackendandOptions, implementing SeaORM 2.x'sMigratorTraitSelf(instance methods) instead of the staticMigratorTraitLazyLockglobals,task_local!statics,init_storage()(withblock_on),MigrationWithData::new(), andrun_with_test()are all eliminatedDatabase::migrate(),refresh(),bootstrap()now take a&Migrator; CLI commands and tests construct migrators directlyMotivation
Data migrations need runtime state (storage backend, concurrency options) passed into their
up/downmethods. SeaORM 1.x'sMigratorTrait::migrations()was a static method with noself, so the project usedLazyLockglobals andtask_local!statics as a side channel to smuggle state into migrations. SeaORM 2.x addedMigratorTraitSelfwithfn migrations(&self), making this workaround unnecessary.Test plan
cargo xtask precommitpasses (schemas, openapi, clippy, fmt, check)cargo test -p migration)trustd db migratestill works with explicit storage config🤖 Generated with Claude Code
Summary by Sourcery
Refactor database migrations to use explicit, instance-based migrator state instead of global migration configuration.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: